Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify simple template #74

Merged
merged 7 commits into from
Sep 4, 2024
Merged

Simplify simple template #74

merged 7 commits into from
Sep 4, 2024

Conversation

joshka
Copy link
Member

@joshka joshka commented Aug 26, 2024

This will flow through to the async template too once the various issues here are resolved.

Copy link
Member Author

@joshka joshka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments rationalizing the decisions here.

simple-generated/Cargo.toml Show resolved Hide resolved
simple-generated/Cargo.toml Show resolved Hide resolved
simple-generated/src/app.rs Outdated Show resolved Hide resolved
simple-generated/src/event.rs Outdated Show resolved Hide resolved
simple-generated/src/event.rs Outdated Show resolved Hide resolved
simple-generated/src/event.rs Outdated Show resolved Hide resolved
simple-generated/src/handler.rs Outdated Show resolved Hide resolved
simple-generated/src/tui.rs Outdated Show resolved Hide resolved
simple-generated/src/ui.rs Outdated Show resolved Hide resolved
simple-generated/src/main.rs Show resolved Hide resolved
@joshka joshka requested a review from orhun August 26, 2024 22:29
@joshka
Copy link
Member Author

joshka commented Aug 26, 2024

I wonder whether this should be simplified further, to a point where it doesn't have the counter code. This way it would make a useful first step on the counter tutorial.

Copy link
Member

@orhun orhun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not super happy with this, I think having separate modules would work better.

This simplifies the template a lot I think.

simple-generated/Cargo.toml Show resolved Hide resolved
simple-generated/src/app.rs Outdated Show resolved Hide resolved
simple-generated/src/main.rs Outdated Show resolved Hide resolved
@joshka
Copy link
Member Author

joshka commented Aug 31, 2024

Where do the comments on this leave us? I suggest this is significantly better than what was there before and should be merged. Are there any comments that are hard blockers @orhun @kdheepak ?

@orhun
Copy link
Member

orhun commented Sep 1, 2024

This template still leaves some beginner questions unanswered and might leave the user confused:

  • How do you send custom events if needed? This is a common issue.

    • We should consider exposing mpsc::Sender somewhere in the app. Alternatively, we could show how to use send().
  • What is color_eyre? I came to use ratatui, but now there's another dependency I need to learn about!

    • We could either remove color_eyre or provide documentation explaining its purpose.
  • The ui() function tends to become very long after a while. Perhaps we could move it to a ui.rs file? This might make it easier to locate and search for.

  • Why not use a constant for FPS?

@joshka
Copy link
Member Author

joshka commented Sep 1, 2024

How do you send custom events if needed? This is a common issue.

In the simple template, you don't. Put another way, the simple template should be a good template for basing the tutorials on. I'd actually like to remove the counter code and make it just a hello world instead. That way our hello world quick start tutorial becomes:

  1. install cargo generate
  2. cargo generate ratatui/templates
  3. cargo run

What is color_eyre? I came to use ratatui, but now there's another dependency I need to learn about!

We do document this, in the second counter tutorial, in https://ratatui.rs/recipes/apps/color-eyre/, in https://github.com/ratatui/ratatui/blob/main/examples/README.md#design-choices (documenting why we use this in all the examples). Additionally hovering over the color_eyre line shows a decent message that answers the question. Including color-eyre in the default template is an opinionated default. It's there because the value it adds when something fails outweighs any downsides.

I'll move everything under simple/template and add a readme to this to help.

The ui() function tends to become very long after a while. Perhaps we could move it to a ui.rs file? This might make it easier to locate and search for.

I have a strong (evidence based) opinion against pre-emptively organizing a project into horizontal layers. Having worked on many projects small and large, horizontal segmentation leads to poorer long term maintainability for a variety of reasons. Doing this would be pre-emptive. Put another way, if I was using the simple template personally, having the extra file would be an annoyance that I'd always want to revert. Reverting would be annoying. Refactoring in the other direction is fairly simple though - highlight code, extract to module, done.

Why not use a constant for FPS?

6ba914b

Remove cargo.lock as this will get out of sync with any dependency bump
@joshka
Copy link
Member Author

joshka commented Sep 1, 2024

Moved the template under simple/template so that we can have a readme for the template and a generated readme for the template user.

Added documentation for color-eyre, fixed up the readme outline of the files, added github workflows and dependabot settings.

Removed cargo.lock as it will get out of sync pretty quickly and running cargo on the generated source will effectively update to the latest immediately.

@orhun
Copy link
Member

orhun commented Sep 2, 2024

I'm not sure adding GitHub workflows to the templates. It might increase complexity, especially for new users who would need to understand what those 100 lines of opinionated YAML is doing, how to use GitHub Actions, etc. Additionally, it's not directly related to the context of TUIs as well. Personally, I’d find it annoying and would likely delete the .github folder before starting to work on the template. Thoughts? @kdheepak @joshka

@joshka
Copy link
Member Author

joshka commented Sep 2, 2024

This is a template, stored on github to create an application. Generating github workflows to ensure that the project builds well is a useful thing to do. Anyone using GitHub should have a basic understanding of github workflows, which are well documented.

The "opinionated" parts of these are fairly insignificant and the sort that I wonder why you'd delete. They just check formatting, check clippy lints, check docs, and run any tests. These are basic hygiene things for any rust project.

Given that deleting these is a matter of deleting a single file, and that this generally makes any project better, I'm for leaving these in.

@kdheepak
Copy link
Contributor

kdheepak commented Sep 3, 2024

I made a couple of minor comments but it looks good to me overall.

@orhun
Copy link
Member

orhun commented Sep 4, 2024

The "opinionated" parts of these are fairly insignificant and the sort that I wonder why you'd delete. They just check formatting, check clippy lints, check docs, and run any tests. These are basic hygiene things for any rust project.

It just felt a bit too much for a simple project creation, that's all. But yes, I agree with your point that the user can just delete them, so it shouldn't be a big deal.

Copy link
Member

@orhun orhun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slapping an approval here not to block this further, might come back to it later :)

Copy link
Member Author

@joshka joshka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the counter behavior and the events module entirely as it was not a "simple" implementation and It's not really necessary for simple apps at all to worry about a tick event. This makes this a good basis for the simple tutorials.

simple-generated/src/event.rs Show resolved Hide resolved
Comment on lines 25 to 37
pub struct EventHandler {
/// Event sender channel.
sender: mpsc::Sender<Event>,
pub struct EventSource {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dislike EventHandler here as the application code needs to handle events somewhere. Event handler would not be that place. The missing info in the prompt is that the event handler both handles the event and sends the tick event. I'm inclined to perhaps actually simplify this further and remove events as used here altogether.

Comment on lines 25 to 37
pub struct EventHandler {
/// Event sender channel.
sender: mpsc::Sender<Event>,
pub struct EventSource {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the entire module in 6a76940

@joshka
Copy link
Member Author

joshka commented Sep 4, 2024

Let's KISS on this template, and move the more opinionated ideas about event handling, threads, etc. to another more advanced template that documents them well.

@joshka
Copy link
Member Author

joshka commented Sep 4, 2024

Merging this to be able to rewrite the getting started docs.

@joshka joshka merged commit ff1be0c into main Sep 4, 2024
3 checks passed
@joshka joshka deleted the jm/simple-init branch September 4, 2024 18:10
@orhun
Copy link
Member

orhun commented Sep 4, 2024

Sounds good, thanks for your work on this 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants